Lower-level support for long-running cohttp-async connections #704

brendanlong · 2020-08-13T22:46:10Z

This adds Cohttp_async.Client.Connection which allows you to open
a connection and re-use it for multiple requests.

Unlike callv, this makes it easy to handle errors and correlate
requests and responses.

brendanlong · 2020-08-13T22:48:39Z

I'm opened this as a PR to give an example of the interface I'm looking for, but I'm open for suggestions if there's a better way to do this.

Cohttp_async.Client.callv doesn't work for me because I need arbitrary-length connections (basically trying to keep a connection open until the server closes it), but callv doesn't give me any way to catch the error when the server closes the connection (the pipe just locks up).

One piece I'm still trying to figure out is if there's any way to detect that the server has disconnected before sending subsequent requests. The Reader.close_finished and Writer.consumer_left events don't seem to trigger until after I made another request.

brendanlong · 2020-08-13T23:21:01Z

(I'm also happy to match any style guidelines but I'm not sure what they are for this project)

brendanlong · 2020-08-13T23:28:28Z

In the internal project I'm working on, I'm attempting a request, catching the "Connection closed by remote host" exception and then retrying once:

  let rec loop ~is_first_request =
    Monitor.try_with (fun () ->
        let%bind connection =
          let key = Scheme_host_port.of_uri target in
          match Hashtbl.find t.open_connections key with
          | Some connection when not (Cohttp_async.Client.Connection.is_closed connection)
            -> return connection
          | _ ->
            let%bind ssl_config = ssl_config ?hostname:(Uri.host target) () in
            let%map connection =
              Cohttp_async.Client.Connection.connect ~ssl_config target
            in
            Hashtbl.set t.open_connections ~key ~data:connection;
            connection
        in
        Cohttp_async.Client.Connection.request ~body connection req)
    >>= function
    | Ok response -> return response
    | Error e ->
      (* We have no way of detecting if the remote side closed a connection before we were able to make a request,
         so always make a request and retry once if the connection was closed *)
      let remote_connection_closed =
        match Monitor.extract_exn e with
        (* This exception is thrown by Cohttp if the other end hangs up *)
        | Failure msg when is_first_request && msg = "Connection closed by remote host" ->
          true
          (* This exception is thrown if multiple requests are queued when the connection is closed *)
        | e when Exn.to_string e |> String.is_substring ~substring:"throttle aborted job"
          -> true
        | _ -> false
      in
      if remote_connection_closed
      then (
        Log.Global.info
          "Remote connection to %s was closed, opening a new one"
          (Uri.to_string target);
        loop ~is_first_request:false)
      else (
        Log.Global.error "%s" (Exn.to_string e);
        Exn.reraisef
          e
          !"Failed to connect to '%s' with request: %{sexp:Request.t}"
          (Uri.to_string uri)
          req
          ())
  in
  loop ~is_first_request:true

brendanlong · 2020-08-18T14:08:15Z

I currently have code similar to this inlined in my Blue_http client but I would rather upstream this piece if I can since I need pieces of Cohttp_async that aren't publish (Cohttp_async.Client.Net): https://github.com/brendanlong/blue-http/blob/master/src/connection.ml

mseri · 2020-08-27T19:21:10Z

Sorry for the delay on this PR.
Gosh, I wish there was a more elegant and robust way than pattern matching on the error message string. Maybe this could become easier once we move to conduit 3.0 (see the on_handler_error function in #692), or maybe not.

mseri · 2020-08-27T19:25:39Z

@avsm I have never used async, I don't feel in the position to properly review this change. Can you recommend somebody more familiar with it for the review?

brendanlong · 2020-09-01T19:58:05Z

The "connection closed by remote host" exception is throw by Cohttp, so we could replace that with a more explicit (not-Failure) exception if you want to: https://github.com/mirage/ocaml-cohttp/blob/master/cohttp-async/src/client.ml#L53

mseri · 2020-10-22T08:30:57Z

If you want to rebase this on master we can try to have it merged before cohttp-3 is merged, or have it ready for a bugfix chottp 3.1 release at least

This adds Cohttp_async.Client.Connection which allows you to open a connection and re-use it for multiple requests. Unlike callv, this makes it easy to handle errors and correlate requests and responses.

brendanlong · 2020-10-22T14:15:00Z

Ok I rebased this

mseri · 2020-10-22T14:26:23Z

Thanks. Yesterday I spent some time to understand what you were doing. It took a while to wrap my head around async but I believe the patch is sound. Is there any test worth to add?

Besides that. Do you still need to catch the "Connection closed by remote host" exception in the same way when using the new cohttp on the master branch?

mseri · 2020-10-22T14:28:28Z

In #692 there was this suggestion concerning the issue:

do you think that the new conduit and this revamp of cohttp can solve the issue pointed out here: #704?

From my perspective, the ability to use something else (defined by the user) than a simple socket such as, in this case, a pool of socket/connection is possible - but it's not well documented and it's not the part of conduit to propose such implementation. The initial issue should be fixed in a different path where the user should implement its own protocol, register it into conduit and re-extract/de-abstract it as well in callv.

So it will not fix the initial issue (where conduit provides something simpler/easier to maintain) but it unlocks the ability to inject an another (new) kind of connection (with another impl. of connect/recv/send/close) into cohttp independently than conduit and cohttp.

So for me, we should keep #704 and may be rethink it in the scope of conduit3.0.0.

The reason I am asking is that exposing the Connection module is somewhat diverging (again) the interfaces between cohttp-async and cohttp-lwt. If a solution using the new capabilities of conduit is possible, that would make it reusable everywhere (so I ping again @dinosaure -- sorry, today you must have received a million emails from me)

dinosaure · 2020-10-23T14:48:43Z

So, I did a deep look into the proposed PR and it seems that the initial error is mostly about the current proposed API of callv. It's hard for me to find a homogenized and safe way to be able to send multiple request into the same connection. My initial idea is to copy the { ic; oc; } - and re-use it with Net.connect_uri with a special ctx which will pass the same { ic; oc; } - but on this way, it will be more complex and may be less understandable that this proposed PR.

The best for me is to accept the PR as is if @brendanlong really needs it 😕 .

mseri · 2020-10-23T14:53:08Z

Thanks for looking into it. I have actually privately heard from other people that would benefit from having this.

@brendanlong if you rebase one last time and add an entry in the CHANGES file I am going to merge

mseri · 2020-10-23T15:45:00Z

Merging, I need to edit the changes file anyways

@emillon

…lwt, cohttp-lwt-unix, cohttp-lwt-unix-ssl, cohttp-top, cohttp-async and cohttp-mirage (3.0.0) CHANGES: - cohttp: update HTTP codes (@emillon mirage/ocaml-cohttp#711) - cohttp: add Uti.t to uri scheme (@brendanlong mirage/ocaml-cohttp#707) - cohttp: fix chunked encoding of empty body (@mefyl mirage/ocaml-cohttp#715) - cohttp-async: fix body not being uploaded with unchunked Async.Pipe (@mefyl mirage/ocaml-cohttp#706) - cohttp-{async, lwt}: fix suprising behaviours of Body.is_empty (@anuragsoni mirage/ocaml-cohttp#714 mirage/ocaml-cohttp#712 mirage/ocaml-cohttp#713) - port to conduit 3.0.0: minor breaking changes on the server API, an explicit distinction between cohttp-lwt-unix (using tls), cohttp-lwt-notls and cohttp-lwt-ssl (using lwt_ssl), and includes ssl in cohttp-async (@dinosaure mirage/ocaml-cohttp#692) - cohttp-lwt-jsoo: rename Cohttp_lwt_xhr to Cohttp_lwt_jsoo for consistency (@mseri mirage/ocaml-cohttp#717) - refactoring of tests (@mseri mirage/ocaml-cohttp#709, @dinosaure mirage/ocaml-cohttp#692) - update documentation (@dinosaure mirage/ocaml-cohttp#716, @mseri mirage/ocaml-cohttp#720) - cohttp: fix transfer-encoding ordering in headers (@mseri mirage/ocaml-cohttp#721) - lower-level support for long-running cohttp-async connections (@brendanlong mirage/ocaml-cohttp#704) - fix deadlock in logging (@dinosaure mirage/ocaml-cohttp#722) - add of_form and to_form functions to body (@seliopou mirage/ocaml-cohttp#440, @mseri mirage/ocaml-cohttp#723) - cohttp-lwt: partly inline read_response, fix body stream leak (@madroach @dinosaure mirage/ocaml-cohttp#696) - improve media type parsing (@seliopou mirage/ocaml-cohttp#542, @dinosaure mirage/ocaml-cohttp#725) - add comparison functions for Request.t and Response.t via ppx_compare (@msaffer-js @dinosaure mirage/ocaml-cohttp#686)

@emillon

…lwt, cohttp-lwt-unix, cohttp-lwt-unix-ssl, cohttp-top, cohttp-async and cohttp-mirage (3.0.0) CHANGES: - cohttp: update HTTP codes (@emillon mirage/ocaml-cohttp#711) - cohttp: add Uti.t to uri scheme (@brendanlong mirage/ocaml-cohttp#707) - cohttp: fix chunked encoding of empty body (@mefyl mirage/ocaml-cohttp#715) - cohttp-async: fix body not being uploaded with unchunked Async.Pipe (@mefyl mirage/ocaml-cohttp#706) - cohttp-{async, lwt}: fix suprising behaviours of Body.is_empty (@anuragsoni mirage/ocaml-cohttp#714 mirage/ocaml-cohttp#712 mirage/ocaml-cohttp#713) - port to conduit 3.0.0: minor breaking changes on the server API, an explicit distinction between cohttp-lwt-unix (using tls), cohttp-lwt-notls and cohttp-lwt-ssl (using lwt_ssl), and includes ssl in cohttp-async (@dinosaure mirage/ocaml-cohttp#692) - cohttp-lwt-jsoo: rename Cohttp_lwt_xhr to Cohttp_lwt_jsoo for consistency (@mseri mirage/ocaml-cohttp#717) - refactoring of tests (@mseri mirage/ocaml-cohttp#709, @dinosaure mirage/ocaml-cohttp#692) - update documentation (@dinosaure mirage/ocaml-cohttp#716, @mseri mirage/ocaml-cohttp#720) - cohttp: fix transfer-encoding ordering in headers (@mseri mirage/ocaml-cohttp#721) - lower-level support for long-running cohttp-async connections (@brendanlong mirage/ocaml-cohttp#704) - fix deadlock in logging (@dinosaure mirage/ocaml-cohttp#722) - add of_form and to_form functions to body (@seliopou mirage/ocaml-cohttp#440, @mseri mirage/ocaml-cohttp#723) - cohttp-lwt: partly inline read_response, fix body stream leak (@madroach @dinosaure mirage/ocaml-cohttp#696) - improve media type parsing (@seliopou mirage/ocaml-cohttp#542, @dinosaure mirage/ocaml-cohttp#725) - add comparison functions for Request.t and Response.t via ppx_compare (@msaffer-js @dinosaure mirage/ocaml-cohttp#686)

@emillon

…lwt, cohttp-lwt-unix, cohttp-lwt-unix-ssl, cohttp-top, cohttp-async and cohttp-mirage (3.0.0) CHANGES: - cohttp: update HTTP codes (@emillon mirage/ocaml-cohttp#711) - cohttp: add Uti.t to uri scheme (@brendanlong mirage/ocaml-cohttp#707) - cohttp: fix chunked encoding of empty body (@mefyl mirage/ocaml-cohttp#715) - cohttp-async: fix body not being uploaded with unchunked Async.Pipe (@mefyl mirage/ocaml-cohttp#706) - cohttp-{async, lwt}: fix suprising behaviours of Body.is_empty (@anuragsoni mirage/ocaml-cohttp#714 mirage/ocaml-cohttp#712 mirage/ocaml-cohttp#713) - port to conduit 3.0.0: minor breaking changes on the server API, an explicit distinction between cohttp-lwt-unix (using tls), cohttp-lwt-notls and cohttp-lwt-ssl (using lwt_ssl), and includes ssl in cohttp-async (@dinosaure mirage/ocaml-cohttp#692) - cohttp-lwt-jsoo: rename Cohttp_lwt_xhr to Cohttp_lwt_jsoo for consistency (@mseri mirage/ocaml-cohttp#717) - refactoring of tests (@mseri mirage/ocaml-cohttp#709, @dinosaure mirage/ocaml-cohttp#692) - update documentation (@dinosaure mirage/ocaml-cohttp#716, @mseri mirage/ocaml-cohttp#720) - cohttp: fix transfer-encoding ordering in headers (@mseri mirage/ocaml-cohttp#721) - lower-level support for long-running cohttp-async connections (@brendanlong mirage/ocaml-cohttp#704) - fix deadlock in logging (@dinosaure mirage/ocaml-cohttp#722) - add of_form and to_form functions to body (@seliopou mirage/ocaml-cohttp#440, @mseri mirage/ocaml-cohttp#723) - cohttp-lwt: partly inline read_response, fix body stream leak (@madroach @dinosaure mirage/ocaml-cohttp#696) - improve media type parsing (@seliopou mirage/ocaml-cohttp#542, @dinosaure mirage/ocaml-cohttp#725) - add comparison functions for Request.t and Response.t via ppx_compare (@msaffer-js @dinosaure mirage/ocaml-cohttp#686)

@emillon

…lwt, cohttp-lwt-unix, cohttp-lwt-unix-ssl, cohttp-top, cohttp-async and cohttp-mirage (3.0.0) CHANGES: - cohttp: update HTTP codes (@emillon mirage/ocaml-cohttp#711) - cohttp: add Uti.t to uri scheme (@brendanlong mirage/ocaml-cohttp#707) - cohttp: fix chunked encoding of empty body (@mefyl mirage/ocaml-cohttp#715) - cohttp-async: fix body not being uploaded with unchunked Async.Pipe (@mefyl mirage/ocaml-cohttp#706) - cohttp-{async, lwt}: fix suprising behaviours of Body.is_empty (@anuragsoni mirage/ocaml-cohttp#714 mirage/ocaml-cohttp#712 mirage/ocaml-cohttp#713) - port to conduit 3.0.0: minor breaking changes on the server API, an explicit distinction between cohttp-lwt-unix (using tls), cohttp-lwt-notls and cohttp-lwt-ssl (using lwt_ssl), and includes ssl in cohttp-async (@dinosaure mirage/ocaml-cohttp#692) - cohttp-lwt-jsoo: rename Cohttp_lwt_xhr to Cohttp_lwt_jsoo for consistency (@mseri mirage/ocaml-cohttp#717) - refactoring of tests (@mseri mirage/ocaml-cohttp#709, @dinosaure mirage/ocaml-cohttp#692) - update documentation (@dinosaure mirage/ocaml-cohttp#716, @mseri mirage/ocaml-cohttp#720) - cohttp: fix transfer-encoding ordering in headers (@mseri mirage/ocaml-cohttp#721) - lower-level support for long-running cohttp-async connections (@brendanlong mirage/ocaml-cohttp#704) - fix deadlock in logging (@dinosaure mirage/ocaml-cohttp#722) - add of_form and to_form functions to body (@seliopou mirage/ocaml-cohttp#440, @mseri mirage/ocaml-cohttp#723) - cohttp-lwt: partly inline read_response, fix body stream leak (@madroach @dinosaure mirage/ocaml-cohttp#696) - improve media type parsing (@seliopou mirage/ocaml-cohttp#542, @dinosaure mirage/ocaml-cohttp#725) - add comparison functions for Request.t and Response.t via ppx_compare (@msaffer-js @dinosaure mirage/ocaml-cohttp#686)

@mseri

…ohttp-top, cohttp-async and cohttp-mirage (4.0.0) CHANGES: - cohttp.response: fix malformed status header for custom status codes (@mseri @aalekseyev mirage/ocaml-cohttp#752) - Remove dependency to base (@samoht mirage/ocaml-cohttp#745) - fix opam files and dependencies - add GitHub Actions workflow (@smorimoto mirage/ocaml-cohttp#739) - lwt_jsoo: Forward exceptions to caller when response is null (@mefyl mirage/ocaml-cohttp#738) - Remove wrapped false (@rgrinberg mirage/ocaml-cohttp#734) - Use implicit executable dependency for generate.exe (@TheLortex mirage/ocaml-cohttp#735) - cohttp: update HTTP codes (@emillon mirage/ocaml-cohttp#711) - cohttp: add Uti.t to uri scheme (@brendanlong mirage/ocaml-cohttp#707) - cohttp: fix chunked encoding of empty body (@mefyl mirage/ocaml-cohttp#715) - cohttp-async: fix body not being uploaded with unchunked Async.Pipe (@mefyl mirage/ocaml-cohttp#706) - cohttp-{async, lwt}: fix suprising behaviours of Body.is_empty (@anuragsoni mirage/ocaml-cohttp#714 mirage/ocaml-cohttp#712 mirage/ocaml-cohttp#713) - cohttp-lwt-jsoo: rename Cohttp_lwt_xhr to Cohttp_lwt_jsoo for consistency (@mseri mirage/ocaml-cohttp#717) - refactoring of tests (@mseri mirage/ocaml-cohttp#709, @dinosaure mirage/ocaml-cohttp#692) - update documentation (@dinosaure mirage/ocaml-cohttp#716, @mseri mirage/ocaml-cohttp#720) - cohttp: fix transfer-encoding ordering in headers (@mseri mirage/ocaml-cohttp#721) - lower-level support for long-running cohttp-async connections (@brendanlong mirage/ocaml-cohttp#704) - fix deadlock in logging (@dinosaure mirage/ocaml-cohttp#722) - add of_form and to_form functions to body (@seliopou mirage/ocaml-cohttp#440, @mseri mirage/ocaml-cohttp#723) - cohttp-lwt: partly inline read_response, fix body stream leak (@madroach @dinosaure mirage/ocaml-cohttp#696) - improve media type parsing (@seliopou mirage/ocaml-cohttp#542, @dinosaure mirage/ocaml-cohttp#725) - add comparison functions for Request.t and Response.t via ppx_compare (@msaffer-js @dinosaure mirage/ocaml-cohttp#686) - [reverted] breaking changes to client and server API to use conduit 3.0.0 (@dinosaure mirage/ocaml-cohttp#692). However, as the design discussion did not reach consensus, these changes were reverted to preserve better compatibility with existing cohttp users. (mirage/ocaml-cohttp#741, @samoht)

@mseri

…ohttp-top, cohttp-async and cohttp-mirage (4.0.0) CHANGES: - cohttp.response: fix malformed status header for custom status codes (@mseri @aalekseyev mirage/ocaml-cohttp#752) - Remove dependency to base (@samoht mirage/ocaml-cohttp#745) - fix opam files and dependencies - add GitHub Actions workflow (@smorimoto mirage/ocaml-cohttp#739) - lwt_jsoo: Forward exceptions to caller when response is null (@mefyl mirage/ocaml-cohttp#738) - Remove wrapped false (@rgrinberg mirage/ocaml-cohttp#734) - Use implicit executable dependency for generate.exe (@TheLortex mirage/ocaml-cohttp#735) - cohttp: update HTTP codes (@emillon mirage/ocaml-cohttp#711) - cohttp: add Uti.t to uri scheme (@brendanlong mirage/ocaml-cohttp#707) - cohttp: fix chunked encoding of empty body (@mefyl mirage/ocaml-cohttp#715) - cohttp-async: fix body not being uploaded with unchunked Async.Pipe (@mefyl mirage/ocaml-cohttp#706) - cohttp-{async, lwt}: fix suprising behaviours of Body.is_empty (@anuragsoni mirage/ocaml-cohttp#714 mirage/ocaml-cohttp#712 mirage/ocaml-cohttp#713) - cohttp-lwt-jsoo: rename Cohttp_lwt_xhr to Cohttp_lwt_jsoo for consistency (@mseri mirage/ocaml-cohttp#717) - refactoring of tests (@mseri mirage/ocaml-cohttp#709, @dinosaure mirage/ocaml-cohttp#692) - update documentation (@dinosaure mirage/ocaml-cohttp#716, @mseri mirage/ocaml-cohttp#720) - cohttp: fix transfer-encoding ordering in headers (@mseri mirage/ocaml-cohttp#721) - lower-level support for long-running cohttp-async connections (@brendanlong mirage/ocaml-cohttp#704) - fix deadlock in logging (@dinosaure mirage/ocaml-cohttp#722) - add of_form and to_form functions to body (@seliopou mirage/ocaml-cohttp#440, @mseri mirage/ocaml-cohttp#723) - cohttp-lwt: partly inline read_response, fix body stream leak (@madroach @dinosaure mirage/ocaml-cohttp#696) - improve media type parsing (@seliopou mirage/ocaml-cohttp#542, @dinosaure mirage/ocaml-cohttp#725) - add comparison functions for Request.t and Response.t via ppx_compare (@msaffer-js @dinosaure mirage/ocaml-cohttp#686) - [reverted] breaking changes to client and server API to use conduit 3.0.0 (@dinosaure mirage/ocaml-cohttp#692). However, as the design discussion did not reach consensus, these changes were reverted to preserve better compatibility with existing cohttp users. (mirage/ocaml-cohttp#741, @samoht)

brendanlong force-pushed the blong/long-running-connections branch from 253d971 to 62e0d2f Compare August 13, 2020 22:51

mseri added the Reviewer Needed label Aug 27, 2020

mseri mentioned this pull request Sep 1, 2020

Conduit 3.0.0 #692

Merged

3 tasks

Lower-level support for long-running cohttp-async connections

ce24b30

This adds Cohttp_async.Client.Connection which allows you to open a connection and re-use it for multiple requests. Unlike callv, this makes it easy to handle errors and correlate requests and responses.

brendanlong force-pushed the blong/long-running-connections branch from 62e0d2f to ce24b30 Compare October 22, 2020 14:12

mseri merged commit dff3fab into mirage:master Oct 23, 2020

mseri mentioned this pull request Nov 3, 2020

[new release] cohttp-lwt-unix-nossl, cohttp-lwt-jsoo, cohttp, cohttp-… ocaml/opam-repository#17571

Merged

mseri mentioned this pull request Mar 24, 2021

[new release] cohttp-lwt-jsoo, cohttp, cohttp-lwt, cohttp-lwt-unix, cohttp-top, cohttp-async and cohttp-mirage (4.0.0) ocaml/opam-repository#18385

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lower-level support for long-running cohttp-async connections #704

Lower-level support for long-running cohttp-async connections #704

brendanlong commented Aug 13, 2020

brendanlong commented Aug 13, 2020

brendanlong commented Aug 13, 2020 •

edited

Loading

brendanlong commented Aug 13, 2020

brendanlong commented Aug 18, 2020

mseri commented Aug 27, 2020 •

edited

Loading

mseri commented Aug 27, 2020

brendanlong commented Sep 1, 2020

mseri commented Oct 22, 2020

brendanlong commented Oct 22, 2020

mseri commented Oct 22, 2020 •

edited

Loading

mseri commented Oct 22, 2020 •

edited

Loading

dinosaure commented Oct 23, 2020

mseri commented Oct 23, 2020

mseri commented Oct 23, 2020

Lower-level support for long-running cohttp-async connections #704

Lower-level support for long-running cohttp-async connections #704

Conversation

brendanlong commented Aug 13, 2020

brendanlong commented Aug 13, 2020

brendanlong commented Aug 13, 2020 • edited Loading

brendanlong commented Aug 13, 2020

brendanlong commented Aug 18, 2020

mseri commented Aug 27, 2020 • edited Loading

mseri commented Aug 27, 2020

brendanlong commented Sep 1, 2020

mseri commented Oct 22, 2020

brendanlong commented Oct 22, 2020

mseri commented Oct 22, 2020 • edited Loading

mseri commented Oct 22, 2020 • edited Loading

dinosaure commented Oct 23, 2020

mseri commented Oct 23, 2020

mseri commented Oct 23, 2020

brendanlong commented Aug 13, 2020 •

edited

Loading

mseri commented Aug 27, 2020 •

edited

Loading

mseri commented Oct 22, 2020 •

edited

Loading

mseri commented Oct 22, 2020 •

edited

Loading